Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add html repr #883

Merged
merged 12 commits into from
Jun 29, 2023
Merged

Add html repr #883

merged 12 commits into from
Jun 29, 2023

Conversation

edeno
Copy link
Contributor

@edeno edeno commented Jun 28, 2023

Motivation

Inspired by the handy html reprs provided by dask, xarray, and pandas (see this blog post for some examples), I wanted to create an easy visual representation of the pynwb file that allows users to:

  1. Easily navigate the structure of the nwb file.
  2. Display how to navigate to nwb objects via tooltip (e.g. nwb_file.fields["processing"]["behavior"].fields["data_interfaces"][ "finger_pos" ].fields["starting_time"])
  3. Allow the user to easily copy this code.

This visualization only displays when using a jupyter notebook, but I believe enough users use these tools for it to be of use. While this does overlap with some of the functionality of NWB Widgets, it is lighter-weight, requires no additional installation, and does no plotting. The goal is for the user to figure out what is in the contents of the nwb file and access the data quickly.

The code below should show off the functionality for an arbitrary NWB file, although the copying of the tooltip is still a work in progress.

How to test the behavior?

import pynwb


class NWBDisplay:
    CSS_STYLE = """
    <style>
        .nwb-fields { font-family: "Segoe UI", Roboto, sans-serif; }
        .nwb-fields .field-key { font-weight: bold; }
        .nwb-fields .field-value { color: #00788E; }
        .nwb-fields details > summary { cursor: pointer; }
        .nwb-fields details > summary:hover { color: #0A6EAA; }
    </style>
    """

    JS_SCRIPT = """
    <script>
        function copyToClipboard(text) {
            navigator.clipboard.writeText(text).then(function() {
                console.log('Copied to clipboard: ' + text);
            }, function(err) {
                console.error('Could not copy text: ', err);
            });
        }
        
        document.addEventListener('DOMContentLoaded', function() {
            let fieldKeys = document.querySelectorAll('.nwb-fields .field-key');
            fieldKeys.forEach(function(fieldKey) {
                fieldKey.addEventListener('click', function() {
                    let accessCode = fieldKey.getAttribute('title').replace('Access code: ', '');
                    copyToClipboard(accessCode);
                });
            });
        });
    </script>
    """

    def __init__(self, nwb_obj):
        self.nwb_obj = nwb_obj
        self.fields = nwb_obj.fields

    def _repr_html_(self):
        html_repr = self.CSS_STYLE
        html_repr += self.JS_SCRIPT
        html_repr += "<div class='nwb-wrap'>"
        html_repr += (
            "<div class='nwb-header'><div class='xr-obj-type'>NWB File</div></div>"
        )
        html_repr += self.generate_html_repr(self.fields)
        html_repr += "</div>"
        return html_repr

    def generate_html_repr(self, fields, level=0, access_code=".fields"):
        html_repr = ""

        if isinstance(fields, dict):
            for key, value in fields.items():
                current_access_code = f"{access_code}['{key}']"
                if (
                    isinstance(value, dict)
                    or isinstance(value, list)
                    or hasattr(value, "fields")
                ):
                    html_repr += f'<details><summary style="margin-left: {level * 20}px;" class="nwb-fields field-key" title="{current_access_code}">{key}</summary>'
                    if hasattr(value, "fields"):
                        value = value.fields
                        current_access_code = current_access_code + ".fields"
                    html_repr += self.generate_html_repr(
                        value, level + 1, current_access_code
                    )
                    html_repr += "</details>"
                else:
                    html_repr += f'<div style="margin-left: {level * 20}px;" class="nwb-fields"><span class="field-key" title="{current_access_code}">{key}:</span> <span class="field-value">{value}</span></div>'
        elif isinstance(fields, list):
            for index, item in enumerate(fields):
                current_access_code = f"{access_code}[{index}]"
                html_repr += f'<div style="margin-left: {level * 20}px;" class="nwb-fields"><span class="field-value" title="{current_access_code}">{str(item)}</span></div>'
        else:
            pass

        return html_repr


nwb_file = pynwb.NWBHDF5IO(
    "test.nwb", mode="r"
).read()

NWBDisplay(nwb_file)

Example display

image

@bendichter
Copy link
Contributor

bendichter commented Jun 28, 2023

woah, awesome! Let's make this the default output for containers! @edeno what happens when you add this as a method of Container:

class Container(AbstractContainer):

@edeno
Copy link
Contributor Author

edeno commented Jun 28, 2023

I believe the commit I added in the pull request did this and should be inherited. Not quite sure how to test this though.

@bendichter
Copy link
Contributor

ah, ok. I'll take a look

@bendichter
Copy link
Contributor

@edeno

I got it to work! I fixed two typos and modified the styles bit. Now this is displayed by default in Jupyter:

image

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Patch coverage: 80.00% and project coverage change: -0.04 ⚠️

Comparison is base (2f9ec56) 88.20% compared to head (0043287) 88.17%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #883      +/-   ##
==========================================
- Coverage   88.20%   88.17%   -0.04%     
==========================================
  Files          44       44              
  Lines        9032     9067      +35     
  Branches     2587     2595       +8     
==========================================
+ Hits         7967     7995      +28     
- Misses        752      757       +5     
- Partials      313      315       +2     
Impacted Files Coverage Δ
src/hdmf/container.py 90.62% <80.00%> (-0.51%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bendichter
Copy link
Contributor

The triangles aren't showing up for me, but I still think this looks pretty good

@edeno
Copy link
Contributor Author

edeno commented Jun 28, 2023

Hm, I do think the triangles are quite useful to indicate you can "open a level". Are you still able to go down levels without the triangles?

@bendichter
Copy link
Contributor

Yes, and I changed the styles so the bold indicates expandable. I like the triangles too. I'm not sure why they aren't showing up for me. I'm not an HTML/CSS expert but I'd be happy to run anything you want to try.

I'm using Chrome in a Jupyter notebook on a mac.

@oruebel
Copy link
Contributor

oruebel commented Jun 28, 2023

I'm not sure why they aren't showing up for me.

My guess is that the font you are using is not supported so the browser falls back on a different font. I.e., I suspect the issue is with .nwb-fields { font-family: "Segoe UI", Roboto, sans-serif; }. Segoe UI is I believe a Windows font so it probably doesn't come with Chrome or Firefox. A quick google search suggests Open Sans could potentially be an alternative, but I'm not sure if that font has the triangle symbol.

woah, awesome!

I agree, that's pretty cool.

what happens when you add this as a method of Container:

I think we probably also would want to add a _repr_html_ for class Data or if the representations are the same for both Data and Container then this could be moved to AbstractContainer instead.

@oruebel oruebel added topic: docs Issues related to documentation category: proposal proposed enhancements or new features priority: medium non-critical problem and/or affecting only a small set of users topic: PyNWB Issues related to the use HDMF in PyNWB labels Jun 28, 2023
@oruebel oruebel added this to the Next Release milestone Jun 28, 2023
@bendichter
Copy link
Contributor

I'm still not getting triangles even with the new font

@bendichter
Copy link
Contributor

image

@bendichter
Copy link
Contributor

bendichter commented Jun 28, 2023

I few other minor points:

  1. Currently, the name "NWB File" is hard-coded as the title. I'd like to change this so that any neurodata object can be viewed this way. For example, here is visualization on a lone ElectricalSeries object.

image

This is almost good, but we should dynamically set the text of the title based on the name of the object. I have changed it to the following rule:

if self.__class__.__name__ == "NWBFile":
    nwb_header_text = "NWBFile"
elif self.name == self.__class__.__name__:
    nwb_header_text = self.name
else:
    nwb_header_text = f"{self.name} ({self.__class__.__name__})"
  1. Currently, the values of datasets are printed. This isn't a big problem in most cases. Usually, large datasets are np.ndarray objects that have a reasonable printing strategy, as seen above. If being read from disk, datasets are h5py or zarr arrays, both of which have reasonable printing strategies.

However, there is currently an edge-case when you have large datasets stored in Python native list objects. In this case, the current print function prints every value:

image

This won't be an issue for any dataset that has been written and read back from disk, so I don't think it's a huge issue, but just something to be aware of

Comment on lines 486 to 487
if self.__class__.__name__ == "NWBFile":
nwb_header_text = "NWBFile"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify why this first part of this if statement to check for "NWBFile" as a name is necessary? Isn't this covered by the second part? Also, it seems strange to check for NWBFile in HDMF.

Copy link
Contributor

@bendichter bendichter Jun 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not covered by the second part because the root NWBFile object technically has the name "root".

Another option is to take this clause out let the name title of an NWBFile object be: "root (NWBFile)".

I suppose another option could be to create a method for this and override it in the NWBFile class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let the name title of an NWBFile object be: "root (NWBFile)"

I think that would be fine. We can always customize in PyNWB if necessary. Alternatively you could also check for:

if self.name == 'root'`:
     nwb_header_text = self.__class__.__name__

but to be honest, I would go with consistency here and just let be root (NWBFile)

@oruebel
Copy link
Contributor

oruebel commented Jun 28, 2023

A more nit-picky comment. I would suggest to remove the nwb prefix from variable names.

@edeno
Copy link
Contributor Author

edeno commented Jun 28, 2023

With respect to the triangle, it should just be rendered via the html tag details (reference here), so it shouldn't be anything fancy. For me it works in jupyter lab in both Chrome and Firefox.
@bendichter Does the details tag display the triangle here on the github PR?

TestTest

@bendichter
Copy link
Contributor

image
Yes, it has always worked within GitHub. There must be some strange styling going on in my jupyter notebook

@edeno
Copy link
Contributor Author

edeno commented Jun 28, 2023

Based on this stack overflow, @bendichter can you try the following CSS:

        CSS_STYLE = """
        <style>
            .container-fields {
                font-family: "Open Sans", Arial, sans-serif;
            }
            .container-fields .field-value {
                color: #00788E;
            }
            .container-fields details > summary {
                cursor: pointer;
                display: list-item;
            }
            .container-fields details > summary:hover {
                color: #0A6EAA;
            }
        </style>
        """

@bendichter
Copy link
Contributor

unfortunately that did not do the trick.

@bendichter
Copy link
Contributor

@edeno and @oruebel adding the styling in like this worked for me!

image

@bendichter
Copy link
Contributor

What do you guys think of hiding np arrays behind a details like this?

image

@oruebel
Copy link
Contributor

oruebel commented Jun 29, 2023

What do you guys think of hiding np arrays behind a details like this?

That looks good to me. Looks like all this needs is a unit test and it is good to go

@bendichter
Copy link
Contributor

I think this is a good checkpoint. It provides clear value above what we have before and works reasonably well. I also think there are lots of opportunities for improvements, e.g.

  • nice tablular rendering for DynamicTables
  • using tooltips for the doc string of each attribute
  • smarter handling of datasets

The nice thing about this implementation is that downstream classes can implement their own custom html

@bendichter
Copy link
Contributor

@oruebel what are you envisioning for a test for this feature?

@bendichter
Copy link
Contributor

@oruebel I added the very basic:

def test_repr_html_(self):
    assert isinstance(Container('obj1')._repr_html_(), str)

which ensures that ._repr_html_() runs without error, but does not check for string equivalence.

@oruebel
Copy link
Contributor

oruebel commented Jun 29, 2023

what are you envisioning for a test for this feature?

I think having a test that checks string equivalence for a Container that covers more of the different cases in the function would be great.

@bendichter bendichter requested a review from oruebel June 29, 2023 20:23
@bendichter bendichter merged commit 6043e77 into hdmf-dev:dev Jun 29, 2023
28 checks passed
@oruebel
Copy link
Contributor

oruebel commented Jun 29, 2023

Looks good to me. Thanks for this great new feature.

@rly
Copy link
Contributor

rly commented Jun 29, 2023

Thanks @edeno for the enhancement!

@rly rly mentioned this pull request Jul 6, 2023
3 tasks
@rly rly removed this from the 3.14.0 milestone Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: proposal proposed enhancements or new features priority: medium non-critical problem and/or affecting only a small set of users topic: docs Issues related to documentation topic: PyNWB Issues related to the use HDMF in PyNWB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants